Avoid empty set on multi_cached #490
Conversation
The multi_cached decorator was always writing in the cache the result of the function, even if it was empty. This was causing the following error inside aioredis:
ERROR aiocache.decorators:decorators.py:370 Couldn't set {}, unexpected error
Traceback (most recent call last):
File "/Users/josepcugat/workspace/aiocache/aiocache/decorators.py", line 367, in set_in_cache
ttl=self.ttl,
File "/Users/josepcugat/workspace/aiocache/aiocache/base.py", line 61, in _enabled
return await func(*args, **kwargs)
File "/Users/josepcugat/workspace/aiocache/aiocache/base.py", line 45, in _timeout
return await asyncio.wait_for(func(self, *args, **kwargs), timeout)
File "/Users/josepcugat/.pyenv/versions/3.6.8/lib/python3.6/asyncio/tasks.py", line 358, in wait_for
return fut.result()
File "/Users/josepcugat/workspace/aiocache/aiocache/base.py", line 75, in _plugins
ret = await func(self, *args, **kwargs)
File "/Users/josepcugat/workspace/aiocache/aiocache/base.py", line 300, in multi_set
await self._multi_set(tmp_pairs, ttl=self._get_ttl(ttl), _conn=_conn)
File "/Users/josepcugat/workspace/aiocache/aiocache/backends/redis.py", line 24, in wrapper
return await func(self, *args, _conn=_conn, **kwargs)
File "/Users/josepcugat/workspace/aiocache/aiocache/backends/redis.py", line 140, in _multi_set
await _conn.mset(*flattened)
File "/Users/josepcugat/.pyenv/versions/3.6.8/envs/aiocache/lib/python3.6/site-packages/aioredis/util.py", line 52, in wait_ok
res = await fut
aioredis.errors.ReplyError: ERR wrong number of arguments for 'mset' command
Codecov Report
@@ Coverage Diff @@
## master #490 +/- ##
=======================================
Coverage 99.47% 99.47%
=======================================
Files 11 11
Lines 950 950
Branches 105 105
=======================================
Hits 945 945
Misses 5 5
Continue to review full report at Codecov.
|
| @@ -328,7 +328,7 @@ async def decorator( | |||
| result = await f(*new_args, **kwargs) | |||
| result.update(partial) | |||
|
|
|||
| if cache_write: | |||
| if result and cache_write: | |||
There was a problem hiding this comment.
As a user... with this (not that before it worked as expected) it means that the execution will be executed every time the results are empty, is that the behavior you would expect?
There was a problem hiding this comment.
Good point, in my case I already had something like:
if not params:
return {}So it wouldn't make any difference. But I can understand that in some cases you might want to do some logic even if the params are empty because everything is cached already.
I would be against putting a new configuration to control this (too many already), so there are 2 options:
- Always call the method even if the params are empty. If you don't want to do anything a shortcut can be added at the beginning of the method to just return, so the behaviour can be controlled.
- Never call the method when the params are empty. In this case if I wanted to do something in the method (like logging) I would not have any workaround.
I would go for option 1 then.
There was a problem hiding this comment.
this looks similar to #452. Maybe the 3rd option is what @paulo-raca mentions in the issue of having an "empty" token and write this into Redis (and others). Wdyt?
There was a problem hiding this comment.
Yes, that would work too, since in that case we would be writing the "guard" value internally instead of the empty dict that causes the error.
There was a problem hiding this comment.
I think that discussion relates more to the other decorators, where the expected return value may be None.
For multi_cached though, we should clearly define what an empty dict signifies. I suspect this change is fine and it can be treated as a 'no-cache' for the result, indicating that something failed temporarily and a result couldn't be retrieved. Alternatively, it could simply be an error and a function is expected to always return values for the keys requested. Or it could be treated as a normal result that should be cached.
I'm not sure what the use cases are for this though.
This comment has been minimized.
This comment has been minimized.
|
If you're still interested in getting this merged, we'll need this updated with latest master. I suspect the test could also be moved into tests/ut/test_decorators.py and done with a mock_cache or similar. |
The multi_cached decorator was always writing in the cache the result of the function, even if it was empty. This was causing the following error inside aioredis: